-
Notifications
You must be signed in to change notification settings - Fork 215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement with
keyword
#1685
Implement with
keyword
#1685
Conversation
... as standardized in dhall-lang/dhall-lang#923
Note that this is still missing tutorial documentation and fixes to downstream packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be nice if we could statically ensure that we'll never encounter a With
after normalization. But of course, With
is not the first constructor like that.
@sjakobi: We actually already have a way to ensure that. The |
Good point @Gabriel439! Maybe we should consider using it in places where we handle normalized expressions, such as parts of |
@sjakobi: Yeah, that was the original intention, but we rolled it out initially in a way that minimized disruption. Another example of where the API could offer a better type is in |
Even with #1702 cherry-picked onto this branch, this error message is somewhat suboptimal:
The main problem is that the error message explains the correct usage of Can we improve that error, possibly by applying a similar trick as the dhall-haskell/dhall/src/Dhall/TypeCheck.hs Lines 806 to 818 in b675924
|
... both for `with` and `//` ... as requested by @sjakobi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress! I have more comments though! ;)
BTW I often feel a bit bad when my review suggestions cause you so much work. Would you prefer if I'd try to address my suggestions myself? I could push directly to your branches (as you sometimes do with my PRs) or open PRs against your branch. What would you prefer?
Also please do push back when you feel that my suggestions are disproportionate or otherwise inappropriate.
@sjakobi: There's no need to apologize! I'm happy to address review feedback and I'm not afraid to push back when appropriate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I believe that errors in with
-expressions will still be a challenge for users who haven't read the record desugaring spec. We'll know more when we get their feedback! :)
... for real this time 😅
... and for nested labels, which was missing
CI is green, code and docs look good, so I'll let mergify merge this! 🚀 |
... as standardized in dhall-lang/dhall-lang#923